Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add commit co-authors support #1482

Merged
merged 6 commits into from
Aug 25, 2022
Merged

Add commit co-authors support #1482

merged 6 commits into from
Aug 25, 2022

Conversation

itsluketwist
Copy link
Contributor

Add co_authors property to the Commit object, which parses the commit message for designated co-authors, include a simple test.

… message for designated co-authors, include a simple test.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, this looks good already!

I have some notes, but when clarified it should be ready for merging.

git/objects/commit.py Outdated Show resolved Hide resolved
test/test_commit.py Outdated Show resolved Hide resolved
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

The only remaining question pertains to the regex used, but once clarified this is ready to go.
Maybe you could also fix flake8 by ignoring this new check that makes CI fail, if you feel comfortable.

@@ -752,11 +752,11 @@ def co_authors(self) -> List[Actor]:

if self.message:
results = re.findall(
r"^Co-authored-by: ((?:\w|\-| ){0,38}) <(\S*)>$",
r"^Co-authored-by: ((?:\w|\-| ){0,38} <\S*>)$",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, can you explain where this regex is coming from? Earlier I tried to argue that the one used by Actor could be used here. It's much more open than this, but I can be swayed if there is a specification of the Co-authored-by: field that limits it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad! Regarding the name part, this is on me, I used a regex to match against the GitHub username specs, but this is just the account name, so a more generic one is necessary - switched to match the regex used in Actor (with the extra Co-authored-by: text and start/end line checks).

Also added a line to ignore the flake8 check - not used flake8 but I think this should be right.

@Byron Byron added this to the v3.1.28 - Bugfixes milestone Aug 25, 2022
@Byron Byron merged commit 73bde1f into gitpython-developers:main Aug 25, 2022
@Byron
Copy link
Member

Byron commented Aug 25, 2022

Thanks a lot for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants